Skip to content

Fix useTable isReady reverting to false after first row event#4580

Open
clockwork-labs-bot wants to merge 1 commit intomasterfrom
bot/fix-usetable-isready-stale
Open

Fix useTable isReady reverting to false after first row event#4580
clockwork-labs-bot wants to merge 1 commit intomasterfrom
bot/fix-usetable-isready-stale

Conversation

@clockwork-labs-bot
Copy link
Collaborator

Fixes #4559.

Bug

The subscribe callback passed to useSyncExternalStore captures computeSnapshot in its closure but did not list it as a dependency. When subscribeApplied flips to true:

  1. computeSnapshot is recreated with subscribeApplied = true
  2. But subscribe still holds the stale closure that captured subscribeApplied = false
  3. On the next row event, the stale computeSnapshot writes [rows, false] into lastSnapshotRef
  4. isReady drops to false permanently

Fix

Add computeSnapshot to the subscribe dependency array so event handlers always use the current snapshot function.

Note: PR #4499 previously fixed a related issue where the cached snapshot was stale after subscribeApplied changed. This fix addresses the remaining case where the subscribe closure itself was stale.

The subscribe callback passed to useSyncExternalStore captured
computeSnapshot in its closure but did not list it as a dependency.
When subscribeApplied flipped to true, computeSnapshot was recreated
with the new value, but subscribe still held the stale closure that
permanently captured subscribeApplied = false.

Adding computeSnapshot to subscribe's dependency array ensures the
event handlers always call the current computeSnapshot, so isReady
stays true after onApplied fires.
Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

connectionState,
accessorName,
querySql,
computeSnapshot,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice we explicitly disable the lint to check that the dependency array is exhaustive, so I'm a little wary of making innocuous-looking changes. I'm not sure who put that there (or if it was AI).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: React SKD useTable isReady reverts to false after first row event

2 participants